Approvers:

• myakove
• rnetser

Reviewers:

• myakove
• rnetser

• hold
• verified
• wip
• lgtm
• approve
• automerge

• Conventional Title: Mode: fix (claude/claude-opus-4-6-1m)
• Cherry-Pick Conflict Resolution: Enabled (claude/claude-opus-4-6-1m)

• Suspicious Path Detection: Monitors paths: .claude/, .vscode/, .cursor/, .devcontainer/, .pi/, .github/workflows/, .github/actions/
• Committer Identity Check: Verifies last committer matches PR author
• Mandatory: Security checks block merge (use /security-override to bypass — maintainers only)

❌ Failed checks (3 warnings)

• X
• Mastodon
• Reddit
• LinkedIn

• Add optional aap_job_template_id to Hook for AAP/AWX-backed hooks.
• Branch Hook.to_dict() to emit spec.aap.jobTemplateId when provided.
• Preserve existing image/playbook Hook spec when AAP job template is unset.

graph TD
  A["Caller/Test"] --> B["Hook.__init__"] --> C{"aap_job_template_id set?"}
  C -- "yes" --> D["Hook spec: aap.jobTemplateId"]
  C -- "no" --> E["Hook spec: image"] --> F["Hook spec: playbook"]

Loading

The following are alternative approaches to this PR:

1. Validate mutual exclusivity in __init__

• ➕ Fails fast when both AAP and image/playbook inputs are provided
• ➕ Makes API contract explicit and avoids ambiguous spec generation
• ➖ Potential behavior change for any callers currently passing both (even if unintended)

2. Introduce a dedicated AAPHook subclass/factory

• ➕ Cleaner separation of the two hook modes and their required fields
• ➕ Easier to extend AAP-specific options later (credentials, controller URL, etc.)
• ➖ More surface area (new class/function) for a small feature
• ➖ May be overkill if only one AAP field is needed short-term

3. Allow passing a raw spec override (advanced usage)

• ➕ Maximum flexibility without adding parameters for every new spec field
• ➕ Useful for future MTV Hook spec expansions
• ➖ Weaker typing/validation; easier to build invalid CRs
• ➖ Harder to keep consistent behavior across callers

Recommendation: The current approach (optional aap_job_template_id with a to_dict() branch) is a good, low-risk, backward-compatible way to unblock AAP integration. Consider adding a simple mutual-exclusion validation (and ideally a small unit test) to make the ‘AAP vs image/playbook’ contract explicit and prevent accidental mixed inputs.

Enhancement (1) +18 / -6

hook.pyAdd AAP job template option and branch Hook spec serialization +18/-6

Add AAP job template option and branch Hook spec serialization

• Adds an optional aap_job_template_id parameter to Hook and stores it on the instance. Updates to_dict() to emit spec.aap.jobTemplateId when set, otherwise preserves the existing spec.image/spec.playbook output.

ocp_resources/hook.py

Description

Hook.__init__ inserts aap_job_template_id before client, changing the positional argument
order; any existing positional calls that previously passed client (or later args) will now bind
that value to aap_job_template_id and leave client=None. This can lead to incorrect Hook
manifests (e.g., non-serializable jobTemplateId) and unexpected client initialization behavior.

Code

ocp_resources/hook.py[R15-21]

        namespace=None,
        image="quay.io/konveyor/hook-runner:latest",
        playbook=None,
+        aap_job_template_id=None,
        client=None,
        teardown=True,
        yaml_file=None,

Relevance

⭐⭐⭐ High

They fix API contract breaks to avoid runtime misbinding/TypeError (see signature fix in PR #2574).

PR-#2574
PR-#2623

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The constructor signature shows aap_job_template_id was added before client, so any positional
argument previously intended for client will now be assigned to aap_job_template_id.

ocp_resources/hook.py[12-23]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`Hook.__init__` added `aap_job_template_id` in the middle of the positional parameters (between `playbook` and `client`). This breaks backward compatibility for any callers using positional arguments, because the value that used to be `client` will now be interpreted as `aap_job_template_id`.

## Issue Context
This project exposes resource wrapper classes as a library; constructor signatures are part of the public API. Adding a new parameter in the middle is a silent behavioral change for positional callers.

## Fix Focus Areas
- ocp_resources/hook.py[12-24]

## Implementation guidance
- Make `aap_job_template_id` keyword-only by inserting `*` before it, e.g.:
 - `def __init__(..., playbook=None, *, aap_job_template_id=None, client=None, ...)`
 This preserves positional compatibility for existing code while still allowing the new feature.
- Alternatively, move `aap_job_template_id` after `yaml_file`/`delete_timeout` (end of the explicit params) and/or require it to be passed by keyword.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

When aap_job_template_id is set, Hook.to_dict() emits only spec.aap.jobTemplateId and omits
spec.image/spec.playbook even if the caller provided them, with no validation to prevent/flag
mixed configuration. This can silently produce a manifest missing expected fields, making
misconfigurations harder to detect.

Code

ocp_resources/hook.py[R47-61]

+            if self.aap_job_template_id is not None:
+                self.res.update({
+                    "spec": {
+                        "aap": {
+                            "jobTemplateId": self.aap_job_template_id,
+                        },
+                    },
+                })
+            else:
+                self.res.update({
+                    "spec": {
+                        "image": self.image,
+                        "playbook": self.playbook,
+                    },
+                })

Relevance

⭐⭐ Medium

Exclusivity/precedence fixes accepted in PR #2480, but added validations often rejected (e.g., PR
#2520).

PR-#2480
PR-#2520

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

to_dict() branches solely on aap_job_template_id is not None; in that branch it constructs
spec with only aap.jobTemplateId and does not include image/playbook at all.

ocp_resources/hook.py[12-21]
ocp_resources/hook.py[44-61]
PR-#2480

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`Hook` currently accepts `image`/`playbook` and `aap_job_template_id` simultaneously, but `to_dict()` will silently prefer the AAP output and drop `image`/`playbook` from the generated manifest. This makes mixed configuration errors easy to miss.

## Issue Context
The constructor has defaults for `image` and an optional `playbook`, so callers may set these while also experimenting with `aap_job_template_id`. Today this yields an output manifest that discards part of the provided configuration without warning.

## Fix Focus Areas
- ocp_resources/hook.py[25-61]

## Implementation guidance
- Add explicit validation in `__init__` (preferred) or `to_dict()`:
 - If `aap_job_template_id is not None` and (`playbook is not None` or `image` was explicitly set), raise `ValueError` explaining the options are mutually exclusive (or document/encode precedence).
 - Optionally, require `playbook` to be non-None when `aap_job_template_id is None` (if the underlying CRD requires it), but only if you can confirm this requirement.
- Update docstring to clearly state exclusivity/precedence.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools